Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update the cache used for LatestRoundRequested #503

Merged
merged 3 commits into from
Aug 5, 2024

Conversation

augustbleeds
Copy link
Collaborator

No description provided.

@@ -133,9 +134,8 @@ func (c *transmissionsCache) LatestRoundRequested(
c.tdLock.RLock()
defer c.tdLock.RUnlock()
configDigest = c.transmissionDetails.Digest
epoch = c.transmissionDetails.Epoch
round = c.transmissionDetails.Round
err = c.assertTransmissionsNotStale()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got rid of the error check because we don't actually care what the config digest is and asserting an error would actually cause you to transmit more rounds

Comment on lines 134 to 138
c.tdLock.RLock()
defer c.tdLock.RUnlock()
configDigest = c.transmissionDetails.Digest
epoch = c.transmissionDetails.Epoch
round = c.transmissionDetails.Round
err = c.assertTransmissionsNotStale()
epoch = 0
round = 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes look fine - but why is LatestRoundRequested implemented in two places? it seems very easy to make changes in one place but forget to make changes in the other

Copy link
Collaborator Author

@augustbleeds augustbleeds Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

originally i think it was to have a cached response and save time. libocr uses the transmissions_cache to satisfy the ContractTransmitter interface.

now that we know we can always return 0 I think we don't need the underlying method. i'll edit that out

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait, actually the Reader needs to implement it because it must conform to the interface was well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i removed the need to query the chain and to use the lock

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my suggestion is - instead of having zero values implemented twice, why not just call c.reader.LatestRoundRequested and add a note saying that this will always return zero values?

that way you only have hard-coded values in one place rather than two places

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i see what you mean. will do 👍

@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 75%)

See analysis details on SonarQube

@augustbleeds augustbleeds merged commit 8ca85b4 into develop Aug 5, 2024
20 of 21 checks passed
@augustbleeds augustbleeds deleted the augustus.forgot-update-cache branch August 5, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants